Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add IndexTemplate field under elasticsearch field #1069

Merged
merged 1 commit into from
Dec 14, 2022
Merged

Add IndexTemplate field under elasticsearch field #1069

merged 1 commit into from
Dec 14, 2022

Conversation

mrodm
Copy link
Contributor

@mrodm mrodm commented Dec 14, 2022 •
edited
Loading

According to the package-spec data_stream manifest definition, the structure under elasticsearch field should be:

elasticsearch:
  index_template:
    mappings:
      ...
    ingest_pipeline:
      name:

Currently, DataStreamManifest definition does not include index_template field https://github.com/elastic/elastic-package/blob/019ab37/internal/packages/packages.go#L141-L145

This PR adds the index_template key to be compliant with the package-spec. Currently, there is no package defining/using this key in the integrations repository.

This addition is also required if at some point mappings need to be taken into account too.

Relates #1018

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-12-14T13:24:50.322+0000

  • Duration: 34 min 17 sec

Test stats 🧪

Test Results
Failed 0
Passed 869
Skipped 0
Total 869

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link
Collaborator

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (34/34) 💚
Files 67.188% (86/128) 👍
Classes 61.957% (114/184) 👍
Methods 48.768% (376/771) 👍
Lines 31.936% (3406/10665) 👍
Conditionals 100.0% (0/0) 💚

@jsoriano
Copy link
Member

This PR adds the index_template key to be compliant with the package-spec. Currently, there is no package defining/using this key in the integrations repository.

This is used by the APM package, that is not in the integrations repository, see here for example: https://github.com/elastic/apm-server/blob/a705bd788cba5f78d82844dab8e3485745844010/apmpackage/apm/data_stream/app_metrics/manifest.yml#L7

@mrodm
Copy link
Contributor Author

mrodm commented Dec 14, 2022

This PR adds the index_template key to be compliant with the package-spec. Currently, there is no package defining/using this key in the integrations repository.

This is used by the APM package, that is not in the integrations repository, see here for example: https://github.com/elastic/apm-server/blob/a705bd788cba5f78d82844dab8e3485745844010/apmpackage/apm/data_stream/app_metrics/manifest.yml#L7

Right, apm package and also the endpoint package use this key (https://github.com/elastic/endpoint-package/search?q=index_template&type=code). In both repositories, they are using the structure of the manifest in data_streams as defined in package-spec, so that's correct.

elastic-package uses this information to retrieve the pipeline name (elasticsearch.index_template.ingest_pipeline.name) in this function:
https://github.com/elastic/elastic-package/blob/main/internal/packages/packages.go#L257
I tried to look for usages of ingest_pipeline in packages, but I didn't find any. I would say this change is needed so elastic-package gets that field value according to what package-spec defines. If not, I think it will always use the default value.

@jsoriano do you think this change could cause any issue?

@mrodm mrodm marked this pull request as ready for review December 14, 2022 15:55
@mrodm mrodm requested a review from a team December 14, 2022 15:55
@mrodm mrodm self-assigned this Dec 14, 2022
@mrodm mrodm merged commit 5370fcb into elastic:main Dec 14, 2022
@mrodm mrodm deleted the add_missing_field branch December 14, 2022 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsoriano jsoriano jsoriano approved these changes

Assignees

@mrodm mrodm

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mrodm @elasticmachine @jsoriano